[Master]-Inconsistent Validation of Non-Deductible VAT % for Purchase Lines Sharing the Same VAT Identifier - #10521
Conversation
…s sharing the same VAT Identifier (Bug 647053)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR adds a new Error() path to the published Purchase Line validation contract without introducing a new API version. table 39 "Purchase Line" backs already-published routes such as the v2.0 purchaseOrderLine API, so the new CheckNonDedVATPctConsistencyForZeroLine() call can now turn existing POST/PATCH requests into new API error responses when another line on the document shares the VAT identifier but has a different "Non-Deductible VAT %". Published API behavior is part of the stable contract; keep existing routes behavior-compatible, or ship this rule behind a new API version instead of changing it in place. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4 |
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR fixes the Non-Deductible VAT purchase-line case where one line can be set to 0 while another line with the same VAT Identifier keeps a non-zero Non-Deductible VAT %. The existing validation already rejects the reverse order, and the new tests cover both entry orders. The rule itself is correct, but the new table-level call is placed outside the existing handled validation path. SuggestionsS1 - Keep the check inside the existing validation Risk assessment and necessityRisk: This is purchase-line VAT validation, so a wrong change can block purchase documents and API writes for a financial field. The same check was added consistently to W1 and the regional BaseApp Purchase Line copies, and no new BaseApp publisher dependency was added; I verified the existing handled event in Necessity: The scenario is valid: without the change, a document can mix 0 and non-zero Non-Deductible VAT % values under the same VAT Identifier depending on line entry order. The scope is targeted and the tests cover both order directions, but the fix should preserve the existing handled validation contract.
|
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Requesting changes for three high-confidence issues:
S1 - Complete the setup-level fix. ADO bug #647365 explicitly requires preventing creation of the conflicting VAT Posting Setup configuration. This PR changes only Purchase Line validation. NonDedVATImpl.CheckVATPostingSetupChangeIsAllowed still exits for a Do Not Allow setup, so the reported non-deductible/normal setup pair with the same VAT Identifier can still be created. Enforce the invariant when the setup is created, as required by the work item.
S2 - Preserve the handled validation contract. The new zero-percent check runs directly from table 39 after NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed. A subscriber that handles OnBeforeCheckNonDeductibleVATPctIsAllowed(..., IsHandled) can suppress the existing check but still receives this new error. Move the zero-value branch into NonDedVATImpl.CheckNonDeductibleVATPctIsAllowed after the same IsHandled gate rather than duplicating the query, label, and error across 14 table copies.
S3 - Resolve the demonstrated test regression. Twenty-one LegacyTestsBucket1 jobs fail. The concrete failure is CombinedVATAmountLineForTwoPurchLineFirstNonDedVATSecondNormalVAT, which intentionally creates non-deductible and normal VAT lines sharing one VAT Identifier; the new validation now aborts that scenario. Since this work item intentionally changes the invariant, revise the existing W1/AU/IT/RU scenarios to assert the new behavior and add setup-level regression coverage. The PR status check is currently failed for this reason.
This is high-risk financial validation across all BaseApp layers. The two added tests cover both line-entry orders, but the implementation does not complete the linked acceptance scope, bypasses the established extensibility event, and breaks the existing suite.
Predrag Maricic (PredragMaricic)
added
Finance
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
3 participants
AB#647365